fix: error when importing keys #175
Conversation
|
Maybe Contributing.md isn't intended for outside users, but I tried running through the necessary testing and hit several issues that aren't accounted for in the md. First ran into a safe.directory issue. Was able to bypass that by adding Second issue was the service account needing Third issue, it is now asking for Please address these issues for new contributors. |
|
/gcbrun |
There was a problem hiding this comment.
Thanks for the PR @hjoh95
Have you tried import using import blocks? I was unable to reproduce the error using TF version 1.5.7 when testing this example by applying it, deleting the statefile and reimporting using import blocks as shown below:
import {
to = module.example.module.kms.google_kms_key_ring.key_ring
id = "projects/sample-project/locations/global/keyRings/sample-kr"
}
import {
to = module.example.module.kms.google_kms_crypto_key.key_ephemeral[0]
id = "projects/sample-project/locations/global/keyRings/sample-kr/cryptoKeys/one"
}
import {
to = module.example.module.kms.google_kms_crypto_key.key_ephemeral[1]
id = "projects/sample-project/locations/global/keyRings/sample-kr/cryptoKeys/two"
}
|
|
||
| locals { | ||
| keys_by_name = zipmap(var.keys, var.prevent_destroy ? slice(google_kms_crypto_key.key[*].id, 0, length(var.keys)) : slice(google_kms_crypto_key.key_ephemeral[*].id, 0, length(var.keys))) | ||
| keys_by_name = zipmap(var.keys, [for k in var.keys : "${google_kms_key_ring.key_ring.id}/cryptoKeys/${k}"]) |
There was a problem hiding this comment.
Disabling prevent_destroy uses a different resource google_kms_crypto_key.key_ephemeral as opposed to google_kms_key_ring.key_ring so we would still need the conditional.
There was a problem hiding this comment.
It uses the google_kms_crypto_key.key_ephemeral resource instead of the google_kms_crypto_key.key resource, but both use the same google_kms_key_ring.key_ring. The conditional was there to select which key resource to reference. I switched the keys_by_name local to set the values to the resultant resource id of the key (because these are always keyring/cryptoKeys/keyname instead of attempting to slice that out of the key resource. This should eliminate the need for that conditional.
There was a problem hiding this comment.
Asked Gemini to explain the caveats for both methods:
My change:
- This method assumes that the resource IDs follow a predictable pattern.
- If a key does not exist, this will still generate an ID string, but it won’t correspond to a real resource.
- No validation that the constructed IDs are valid or exist.
The previous structure:
- Assumes the resources are created in the same order as var.keys. If the resource creation order doesn’t match, the mapping may be incorrect.
- If the number of resources is less than length(var.keys), you may get errors or missing mappings.
- If resources are created dynamically or filtered, the mapping may not be reliable.
- More robust than the first method, as it uses actual resource IDs, but still depends on ordering.
I'm not sure I agree with the "more robust" claim because of the import issue, but I will concede that my method assumes the resource id instead of pulling it from the resource. I am struggling to think of a scenario where this would be detrimental though.
|
Thanks for the response, I'll take a look at import blocks. My team utilizes Terragrunt heavily, so importing with the single line CLI command is the only import method we're really familiar with. I'll try to take a look at your other comment this weekend. |
|
I've made some changes to make the module less rigid. Using the key name for the index instead of a numeric value allows for a user to insert new keys anywhere in the list without causing resource destruction. Input usage is still the same, but in order to upgrade modules, a user would need to delete state and import keys. Let me know what you think @bharathkkb - I can pull it back if necessary. |
|
@bharathkkb, bump |
3 participants
This should fix #44. Simplified the logic to construct the Google object ids for each key instead of slicing them out of the resource blocks.